perf: cache the resolved cloakbrowser version instead of re-reading it per status call - #213

Description

resolveCloakBrowserVersion() did a module resolution, a synchronous file read, and a JSON parse on every invocation, with no caching — for a value that cannot change while the process is alive.

It is called more often than it looks. status() calls it directly (provider.ts:24), and profileStatuses() calls it inside the .map() (session-manager.ts:126), so once per profile. A single status() with N active profiles performed N+1 resolve-read-parse cycles, all returning the same string. webcmd doctor and every runtime status poll paid that cost; in the daemon, where status is polled repeatedly, it was repeated synchronous I/O on the event loop.

Closes #211

The sentinel

The memo uses a Symbol sentinel rather than a plain if (cached !== undefined) guard:

const UNRESOLVED = Symbol('unresolved');
let cachedCloakBrowserVersion: string | undefined | typeof UNRESOLVED = UNRESOLVED;

undefined is a legitimate result — it is what the catch returns when cloakbrowser cannot be resolved. An undefined-based guard would treat that as "not cached yet" and retry the failing resolve on every single call, preserving the bug precisely in the case where the resolution is most likely to be slow. The sentinel caches the failure too.

Scope

Deliberately left alone:

• profileStatuses() still calls the function inside its .map(). With the cache that is an O(1) lookup per row, and hoisting it would be an unrelated edit to a function this issue is not about.
• No test-only cache reset. The issue floated one as optional. Nothing needs it today: no test calls resolveCloakBrowserVersion, and the ten runtimeVersion occurrences across the suite are all stubbed fixtures. Trivial to add if that changes.

Type of Change

• 🐛 Bug fix
• ✨ New feature
• 🌐 New site adapter
• 📝 Documentation
• ♻️ Refactor
• 🔧 CI / build / tooling

Filed as a bug rather than a refactor: the previous implementation cached this per runtime, and the caching was dropped during the local-cloak refactor. This restores intended behavior.

Checklist

• I ran the checks relevant to this PR
• I updated tests or docs if needed
• I included output or screenshots when useful

Notes on the checklist:

• No observable behavior change. The function returns the same value it did before, including undefined when cloakbrowser is unresolvable — it just stops recomputing it.
• I did not run a full npx tsc --noEmit. It reports src/fetch/client.ts(2,23): Cannot find module 'impit' in my environment — impit@0.14.3 is in dependencies but absent from my node_modules, unrelated to any file here.

Adapter Notes

Not applicable — this is runtime internals, no adapter is added or modified.

• Updated generated or lean docs when command discoverability changed
• Used positional args for the command's primary subject unless a named flag is clearly better
• Normalized expected adapter failures to CliError subclasses instead of raw Error

Screenshots / Output

Full local-cloak suite:

$ npx vitest run --project unit src/browser/runtime/local-cloak/

 Test Files  6 passed (6)
      Tests  67 passed (67)

The new test lives in its own file because the cache is module-level and the first call anywhere in the process fills it. The spy is installed after the import — loading the module graph performs file reads of its own, and an earlier version of this test counted those too:

vi.resetModules();
const fs = (await import('node:fs')).default;
// Import before spying: loading the module graph reads files of its own,
// and only reads made by resolveCloakBrowserVersion should be counted.
const { resolveCloakBrowserVersion } = await import('./session-manager.js');
const readFileSync = vi.spyOn(fs, 'readFileSync');

const first = resolveCloakBrowserVersion();
const second = resolveCloakBrowserVersion();
const third = resolveCloakBrowserVersion();

expect(second).toBe(first);
expect(third).toBe(first);
expect(readFileSync).toHaveBeenCalledTimes(1);

Confirmed the test fails without the fix — stashing the session-manager.ts change and re-running gives:

AssertionError: expected "readFileSync" to be called 1 times, but got 3 times